Fix #47: Support NaN values for disconnected line segments #48

krystophny · 2025-07-20T16:37:50Z

User description

Summary

Implements NaN support in add_plot to allow disconnected line segments
Resolves issue Multiple add_plot calls connect disconnected line segments #47 where multiple add_plot calls would connect separate line segments
Users can now insert NaN values in their data arrays to create line breaks

Implementation Details

The solution modifies the line rendering routines to check for NaN values:

render_solid_line: Skips any segment where either endpoint contains NaN
render_patterned_line: Handles NaN by resetting pattern state and skipping segments
render_markers: Skips data points with NaN values

All backends (PNG, PDF, ASCII) automatically inherit this behavior since they use the same rendering logic.

Test Plan

Added test_disconnected_lines.f90 with two test cases:
- Multiple add_plot calls showing the original issue
- Single add_plot with NaN separators showing the fix
Added example disconnected_lines.f90 demonstrating practical usage
Verified all backends produce correct output with disconnected segments
Existing tests continue to pass

Example Usage

use, intrinsic :: ieee_arithmetic, only: ieee_value, ieee_quiet_nan
real(8) :: x(5), y(5), nan

nan = ieee_value(nan, ieee_quiet_nan)

\! Two segments separated by NaN: (0,0)-(1,0) and (2,1)-(3,1)
x = [0.0_8, 1.0_8, nan, 2.0_8, 3.0_8]
y = [0.0_8, 0.0_8, nan, 1.0_8, 1.0_8]

call fig%add_plot(x, y)

Closes #47

🤖 Generated with Claude Code

PR Type

Bug fix, Enhancement

Description

Implement NaN support for disconnected line segments
Skip NaN values in line rendering and markers
Add comprehensive tests and examples
Support all backends (PNG, PDF, ASCII)

Diagram Walkthrough

flowchart LR
  A["Data with NaN"] --> B["render_solid_line"]
  A --> C["render_patterned_line"]
  A --> D["render_markers"]
  B --> E["Skip NaN segments"]
  C --> F["Reset pattern state on NaN"]
  D --> G["Skip NaN points"]
  E --> H["Disconnected lines"]
  F --> H
  G --> H

File Walkthrough

Relevant files

Bug fix

fortplot_figure_core.f90 `Implement NaN handling in line rendering` src/fortplot_figure_core.f90 Add NaN checks in `render_solid_line` to skip invalid segments Modify `render_patterned_line` to handle NaN with pattern state reset Update `render_markers` to skip NaN data points Import `ieee_arithmetic` module for NaN detection	+37/-6

Tests

test_disconnected_lines.f90 `Add disconnected lines test cases` test/test_disconnected_lines.f90 Add test for multiple plots that shouldn't connect Add test for NaN values breaking line segments Generate PNG outputs for visual verification Use `ieee_arithmetic` for NaN value creation	+56/-0

Documentation

disconnected_lines.f90 `Add disconnected lines usage example` example/disconnected_lines.f90 Create example with three disconnected segments using NaN separators Demonstrate sine, cosine, and horizontal line segments Show single point plotting capability Generate outputs in PNG, PDF, and ASCII formats	+63/-0

Implements NaN support in add_plot to allow disconnected line segments, resolving issue #47. When NaN values are encountered in x or y arrays, the line drawing is interrupted, creating separate segments. Changes: - Modified render_solid_line to skip segments containing NaN values - Updated render_patterned_line to handle NaN with pattern state reset - Enhanced render_markers to skip NaN data points - All backends (PNG, PDF, ASCII) automatically support this feature This allows users to plot disconnected segments in a single add_plot call by inserting NaN values as separators, similar to matplotlib behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

qodo-merge-pro · 2025-07-20T16:38:28Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 47 - PR Code Verified Compliant requirements: • Fix issue where multiple add_plot calls connect disconnected line segments • Support NaN values as line breaks (like matplotlib) • Ensure two separate horizontal line segments remain disconnected • Provide solution for plotting mesh triangles without spurious connections Requires further human verification: • Visual verification that PNG/PDF outputs show properly disconnected segments • Testing with actual mesh triangle plotting use case from FortFEM
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Performance Impact The NaN checking logic is added to every segment rendering loop which could impact performance for large datasets. Consider if the NaN checks could be optimized or cached. if (ieee_is_nan(self%plots(plot_idx)%x(i)) .or. ieee_is_nan(self%plots(plot_idx)%y(i)) .or. & ieee_is_nan(self%plots(plot_idx)%x(i+1)) .or. ieee_is_nan(self%plots(plot_idx)%y(i+1))) then cycle end if Memory Allocation The `render_patterned_line` function allocates additional arrays for valid_points tracking but the cleanup only deallocates some arrays. Verify all allocated memory is properly freed. logical, allocatable :: valid_points(:) ! Transform all data points to get proper scaling allocate(x_trans(size(self%plots(plot_idx)%x))) allocate(y_trans(size(self%plots(plot_idx)%y))) allocate(valid_points(size(self%plots(plot_idx)%x))) valid_count = 0 do i = 1, size(self%plots(plot_idx)%x) valid_points(i) = .not. (ieee_is_nan(self%plots(plot_idx)%x(i)) .or. ieee_is_nan(self%plots(plot_idx)%y(i))) if (valid_points(i)) then x_trans(i) = apply_scale_transform(self%plots(plot_idx)%x(i), self%xscale, self%symlog_threshold) y_trans(i) = apply_scale_transform(self%plots(plot_idx)%y(i), self%yscale, self%symlog_threshold) valid_count = valid_count + 1 else x_trans(i) = 0.0_wp y_trans(i) = 0.0_wp end if end do x_range = maxval(x_trans) - minval(x_trans) y_range = maxval(y_trans) - minval(y_trans) plot_scale = max(x_range, y_range) ! Define pattern lengths (matplotlib-like) dash_len = plot_scale * 0.03_wp ! 3% of range dot_len = plot_scale * 0.005_wp ! 0.5% of range gap_len = plot_scale * 0.015_wp ! 1.5% of range ! Define patterns like matplotlib select case (trim(linestyle)) case ('--') ! Dashed: [dash, gap, dash, gap, ...] pattern_size = 2 pattern(1) = dash_len ! dash pattern(2) = gap_len ! gap case (':') ! Dotted: [dot, gap, dot, gap, ...] pattern_size = 2 pattern(1) = dot_len ! dot pattern(2) = gap_len ! gap case ('-.') ! Dash-dot: [dash, gap, dot, gap, dash, gap, dot, gap, ...] pattern_size = 4 pattern(1) = dash_len ! dash pattern(2) = gap_len ! gap pattern(3) = dot_len ! dot pattern(4) = gap_len ! gap case default ! Unknown pattern, fall back to solid call render_solid_line(self, plot_idx) deallocate(x_trans, y_trans) return end select ! Calculate total pattern length pattern_length = sum(pattern(1:pattern_size)) ! Render with continuous pattern current_distance = 0.0_wp pattern_index = 1 drawing = .true. ! Start drawing do i = 1, size(self%plots(plot_idx)%x) - 1 ! Skip segment if either point is invalid (NaN) if (.not. valid_points(i) .or. .not. valid_points(i+1)) then ! Reset pattern state when encountering NaN current_distance = 0.0_wp pattern_index = 1 drawing = .true. cycle end if x1_screen = x_trans(i) y1_screen = y_trans(i) x2_screen = x_trans(i+1) y2_screen = y_trans(i+1) dx = x2_screen - x1_screen dy = y2_screen - y1_screen segment_length = sqrt(dxdx + dydy) if (segment_length < 1e-10_wp) cycle call render_segment_with_pattern(self, x1_screen, y1_screen, x2_screen, y2_screen, segment_length, & pattern, pattern_size, pattern_length, & current_distance, pattern_index, drawing) end do ! Clean up deallocate(x_trans, y_trans, valid_points)

qodo-merge-pro · 2025-07-20T16:39:08Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Handle all-NaN data gracefully~~ Suggestion Impact: The suggestion was almost directly implemented. The commit adds the same conditional check for valid_count > 0, uses masks for minval/maxval calculations, and provides a default scale value. The only difference is that the commit sets plot_scale directly to 1.0_wp instead of setting x_range and y_range to 1.0_wp separately. code diff: + ! Handle case where all points are NaN + if (valid_count > 0) then + x_range = maxval(x_trans, mask=valid_points) - minval(x_trans, mask=valid_points) + y_range = maxval(y_trans, mask=valid_points) - minval(y_trans, mask=valid_points) + plot_scale = max(x_range, y_range) + else + ! All points are NaN, use default scale + plot_scale = 1.0_wp + end if When all points are NaN, `x_trans` and `y_trans` arrays contain only zeros, making `maxval` and `minval` both zero, resulting in zero ranges. This could cause division by zero or scaling issues later in the code. src/fortplot_figure_core.f90 [1480-1481] -x_range = maxval(x_trans) - minval(x_trans) -y_range = maxval(y_trans) - minval(y_trans) +if (valid_count > 0) then + x_range = maxval(x_trans, mask=valid_points) - minval(x_trans, mask=valid_points) + y_range = maxval(y_trans, mask=valid_points) - minval(y_trans, mask=valid_points) +else + x_range = 1.0_wp + y_range = 1.0_wp +end if `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that `minval` and `maxval` will be calculated on an array containing zeros for NaN points, which can lead to an incorrect plot scale, and provides a robust fix using a mask.	Medium
Possible issue	Fix incorrect array indexing The cosine calculation uses incorrect indexing. The expression `x(i-5+1)` simplifies to `x(i-4)`, which doesn't correspond to the intended x-values for the cosine wave. Use the actual x-values from the array instead. src/fortplot_figure_core.f90 [29-31] do i = 6, 9 - y(i) = cos(x(i-5+1)) + y(i) = cos(x(i)) end do `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a bug in the example code where incorrect indexing `x(i-5+1)` would produce a distorted plot; however, it incorrectly identifies the file as `src/fortplot_figure_core.f90`.	Medium
Update

codecov · 2025-07-20T16:40:17Z

Codecov Report

Attention: Patch coverage is 79.41176% with 35 lines in your changes missing coverage. Please review.

Project coverage is 64.26%. Comparing base (cd503bc) to head (6e468ce).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
example/disconnected_lines.f90	0.00%	31 Missing ⚠️
src/fortplot_figure_core.f90	84.00%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   63.42%   64.26%   +0.84%     
==========================================
  Files         142      144       +2     
  Lines       11493    11658     +165     
  Branches     1971     1977       +6     
==========================================
+ Hits         7289     7492     +203     
+ Misses       3262     3218      -44     
- Partials      942      948       +6

Flag	Coverage Δ
unittests	`64.26% <79.41%> (+0.84%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Add additional test cases to cover: - Patterned lines (dashed) with NaN breaks - Markers-only plots with NaN values This tests more code paths in render_patterned_line and render_markers to improve test coverage. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Add extensive test coverage for NaN handling edge cases: - All line style patterns (solid, dashed, dotted, dash-dot, unknown) - Edge cases: all NaN arrays, leading/trailing NaN, consecutive NaN - Small arrays: single points, two points with NaN combinations - Robust handling when valid_count = 0 using masked maxval/minval Also fix potential crash when all points are NaN by adding safeguard for maxval/minval operations and providing default plot_scale. This should significantly improve code coverage metrics. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Fix bug where cosine calculation used incorrect indexing x(i-5+1) instead of x(i), which would produce incorrect plot values. Addresses review feedback from PR #48. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

* fix: Support NaN values for disconnected line segments Implements NaN support in add_plot to allow disconnected line segments, resolving issue #47. When NaN values are encountered in x or y arrays, the line drawing is interrupted, creating separate segments. Changes: - Modified render_solid_line to skip segments containing NaN values - Updated render_patterned_line to handle NaN with pattern state reset - Enhanced render_markers to skip NaN data points - All backends (PNG, PDF, ASCII) automatically support this feature This allows users to plot disconnected segments in a single add_plot call by inserting NaN values as separators, similar to matplotlib behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: Improve coverage for NaN line breaking Add additional test cases to cover: - Patterned lines (dashed) with NaN breaks - Markers-only plots with NaN values This tests more code paths in render_patterned_line and render_markers to improve test coverage. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: Comprehensive NaN line breaking coverage Add extensive test coverage for NaN handling edge cases: - All line style patterns (solid, dashed, dotted, dash-dot, unknown) - Edge cases: all NaN arrays, leading/trailing NaN, consecutive NaN - Small arrays: single points, two points with NaN combinations - Robust handling when valid_count = 0 using masked maxval/minval Also fix potential crash when all points are NaN by adding safeguard for maxval/minval operations and providing default plot_scale. This should significantly improve code coverage metrics. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Correct array indexing in disconnected_lines example Fix bug where cosine calculation used incorrect indexing x(i-5+1) instead of x(i), which would produce incorrect plot values. Addresses review feedback from PR #48. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add histogram plotting support with TDD * Add PLOT_TYPE_HISTOGRAM constant and hist method to figure_t * Implement histogram binning with automatic and custom bin counts * Support density normalization for probability density plots * Include comprehensive test suite with edge cases * Add example demonstrating various histogram features * Support all backends (PNG, PDF, ASCII) Follows matplotlib-compatible API: fig%hist(data, bins=10, density=.false.) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Break long lines in fortplot_figure module The lines exceeded the 132 character limit causing CI failures. Split the use and public statements across multiple lines. * fix: Address code review feedback for histogram implementation - Add size validation to normalize_histogram_density to prevent array bounds violations - Handle edge case when all data points are identical to avoid zero bin width - Add protection against zero plot scale division in pattern calculations - Optimize find_bin_index to use binary search instead of linear search for O(log n) performance - Add comprehensive edge case tests to verify fixes These improvements address all the valid concerns raised in the PR review. * fix: Update histogram demo to use FORD-compatible output directory and fix branding * fix: refactor histogram routines for SOLID compliance and eliminate magic numbers - Split find_bin_index (34 lines) into three focused functions under 30 lines: * find_bin_index (22 lines): main entry point * is_value_in_range (8 lines): range validation * binary_search_bins (24 lines): binary search logic - Extract magic numbers to named constants: * DEFAULT_HISTOGRAM_BINS = 10 (was hardcoded 10) * IDENTICAL_VALUE_PADDING = 0.5_wp (was hardcoded 0.5_wp) * BIN_EDGE_PADDING_FACTOR = 0.001_wp (was hardcoded 0.001_wp) - Maintain Single Responsibility Principle compliance - All histogram tests pass, functionality preserved * fix: Correct dependency name in fpm example * fix: refactor remaining histogram routines for 30-line SOLID compliance Split remaining large histogram routines into focused sub-routines: - add_histogram_plot_data (62→26 lines): Separated bin setup and properties - render_histogram_plot (45→20 lines): Extracted individual bar rendering - create_histogram_xy_data (39→22 lines): Split outline point generation New helper routines (all ≤30 lines): - setup_histogram_bins (28 lines): Handle bin creation and normalization - setup_histogram_plot_properties (28 lines): Configure label, color, style - render_histogram_bar (10 lines): Coordinate single bar rendering - transform_histogram_bar_coordinates (22 lines): Data to screen transforms - draw_histogram_bar_shape (11 lines): Backend drawing calls - add_bar_outline_points (26 lines): Generate 4-corner bin outline All routines now comply with SOLID single responsibility principle. Functionality and performance preserved exactly. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: expose histogram in main fortplot API for matplotlib compatibility Adds hist() and histogram() subroutines to main fortplot module public interface, enabling matplotlib-style usage: - call hist(data, bins=20, density=.true., label='Distribution') - call histogram(data, bins=20, density=.true., label='Distribution') Both APIs forward all parameters (data, bins, density, label, color) to the existing figure%hist() method, ensuring identical functionality. Resolves CRITICAL API exposure gap where histogram was only available through figure method (fig%hist()) but not through global API like other plot types (plot, contour, etc.). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: refactor hist subroutine for SOLID compliance Extract validation logic into separate helper function to reduce main histogram subroutine from 31 lines to 23 lines, satisfying the mandatory 30-line maximum requirement. This maintains single responsibility principle while preserving all existing functionality. - Add validate_histogram_input helper function - Maintain all error checking and warning functionality - All histogram tests continue to pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address critical security and quality findings - Add input validation for bins parameter in histogram functions - Remove 40 binary artifact files from working directory - Replace warning output pollution with silent error handling - Extract magic numbers to named constants for line width, contour levels, and pattern factors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: mark histogram feature as implemented in README Update checkbox to reflect that histogram functionality is complete and functional in the library API. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: prevent segmentation faults in histogram with invalid bins parameter - Move bins validation from setup_histogram_bins to validate_histogram_input - Validate bins > 0 and bins <= MAX_SAFE_BINS before processing - Prevent plot_count increment when validation fails - Add comprehensive test coverage for boundary conditions - Test both figure.hist() and global hist()/histogram() APIs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: update repository references from krystophny/fortplotlib to lazy-fortran/fortplot Updates CMake configuration, documentation URLs, and example references to use correct repository location for CI/CD pipeline functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: remove binary files and build artifacts from workspace - Remove all test output files (*.png, *.pdf, test_*.txt) - Remove build directories and .mod files - Remove macOS artifact file (._.DS_Store) - Ensure clean workspace with no binary files or build artifacts - .gitignore properly configured to prevent future binary commits 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: update CMake example to use correct fortplot target names - Changed fortplotlib to fortplot in FetchContent_Declare - Updated target link from fortplotlib::fortplotlib to fortplot::fortplot - Aligns with actual CMake targets defined in main CMakeLists.txt - Resolves CMake CI build failure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: correct test executable name in CI and improve histogram plot indexing * fix: move histogram test files from app/ to test/ directory for proper organization * fix: resolve segmentation fault in global pyplot-style histogram functions Address critical issue where global hist() and histogram() functions crashed with segmentation fault due to uninitialized global figure backend. Root cause: Global figure variable declared but never auto-initialized when global functions called methods on it. Solution: - Add ensure_global_figure_initialized() helper function - Auto-initialize global figure with default dimensions if backend not allocated - Call helper before hist operations in both hist() and histogram() functions - Maintain matplotlib compatibility by auto-initializing like pyplot.hist() Changes: - src/fortplot.f90: Add auto-initialization to hist() and histogram() - test/test_global_hist_api.f90: Add test for auto-initialization behavior Verification: - Global hist(data) now works without explicit figure() call - Object-oriented fig%hist() continues working unchanged - All existing tests pass - New test verifies matplotlib-compatible auto-initialization 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>

* fix: Support NaN values for disconnected line segments Implements NaN support in add_plot to allow disconnected line segments, resolving issue #47. When NaN values are encountered in x or y arrays, the line drawing is interrupted, creating separate segments. Changes: - Modified render_solid_line to skip segments containing NaN values - Updated render_patterned_line to handle NaN with pattern state reset - Enhanced render_markers to skip NaN data points - All backends (PNG, PDF, ASCII) automatically support this feature This allows users to plot disconnected segments in a single add_plot call by inserting NaN values as separators, similar to matplotlib behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: Improve coverage for NaN line breaking Add additional test cases to cover: - Patterned lines (dashed) with NaN breaks - Markers-only plots with NaN values This tests more code paths in render_patterned_line and render_markers to improve test coverage. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * test: Comprehensive NaN line breaking coverage Add extensive test coverage for NaN handling edge cases: - All line style patterns (solid, dashed, dotted, dash-dot, unknown) - Edge cases: all NaN arrays, leading/trailing NaN, consecutive NaN - Small arrays: single points, two points with NaN combinations - Robust handling when valid_count = 0 using masked maxval/minval Also fix potential crash when all points are NaN by adding safeguard for maxval/minval operations and providing default plot_scale. This should significantly improve code coverage metrics. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Correct array indexing in disconnected_lines example Fix bug where cosine calculation used incorrect indexing x(i-5+1) instead of x(i), which would produce incorrect plot values. Addresses review feedback from PR #48. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add histogram plotting support with TDD * Add PLOT_TYPE_HISTOGRAM constant and hist method to figure_t * Implement histogram binning with automatic and custom bin counts * Support density normalization for probability density plots * Include comprehensive test suite with edge cases * Add example demonstrating various histogram features * Support all backends (PNG, PDF, ASCII) Follows matplotlib-compatible API: fig%hist(data, bins=10, density=.false.) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Break long lines in fortplot_figure module The lines exceeded the 132 character limit causing CI failures. Split the use and public statements across multiple lines. * fix: Address code review feedback for histogram implementation - Add size validation to normalize_histogram_density to prevent array bounds violations - Handle edge case when all data points are identical to avoid zero bin width - Add protection against zero plot scale division in pattern calculations - Optimize find_bin_index to use binary search instead of linear search for O(log n) performance - Add comprehensive edge case tests to verify fixes These improvements address all the valid concerns raised in the PR review. * fix: Update histogram demo to use FORD-compatible output directory and fix branding * fix: refactor histogram routines for SOLID compliance and eliminate magic numbers - Split find_bin_index (34 lines) into three focused functions under 30 lines: * find_bin_index (22 lines): main entry point * is_value_in_range (8 lines): range validation * binary_search_bins (24 lines): binary search logic - Extract magic numbers to named constants: * DEFAULT_HISTOGRAM_BINS = 10 (was hardcoded 10) * IDENTICAL_VALUE_PADDING = 0.5_wp (was hardcoded 0.5_wp) * BIN_EDGE_PADDING_FACTOR = 0.001_wp (was hardcoded 0.001_wp) - Maintain Single Responsibility Principle compliance - All histogram tests pass, functionality preserved * fix: Correct dependency name in fpm example * fix: refactor remaining histogram routines for 30-line SOLID compliance Split remaining large histogram routines into focused sub-routines: - add_histogram_plot_data (62→26 lines): Separated bin setup and properties - render_histogram_plot (45→20 lines): Extracted individual bar rendering - create_histogram_xy_data (39→22 lines): Split outline point generation New helper routines (all ≤30 lines): - setup_histogram_bins (28 lines): Handle bin creation and normalization - setup_histogram_plot_properties (28 lines): Configure label, color, style - render_histogram_bar (10 lines): Coordinate single bar rendering - transform_histogram_bar_coordinates (22 lines): Data to screen transforms - draw_histogram_bar_shape (11 lines): Backend drawing calls - add_bar_outline_points (26 lines): Generate 4-corner bin outline All routines now comply with SOLID single responsibility principle. Functionality and performance preserved exactly. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: expose histogram in main fortplot API for matplotlib compatibility Adds hist() and histogram() subroutines to main fortplot module public interface, enabling matplotlib-style usage: - call hist(data, bins=20, density=.true., label='Distribution') - call histogram(data, bins=20, density=.true., label='Distribution') Both APIs forward all parameters (data, bins, density, label, color) to the existing figure%hist() method, ensuring identical functionality. Resolves CRITICAL API exposure gap where histogram was only available through figure method (fig%hist()) but not through global API like other plot types (plot, contour, etc.). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: refactor hist subroutine for SOLID compliance Extract validation logic into separate helper function to reduce main histogram subroutine from 31 lines to 23 lines, satisfying the mandatory 30-line maximum requirement. This maintains single responsibility principle while preserving all existing functionality. - Add validate_histogram_input helper function - Maintain all error checking and warning functionality - All histogram tests continue to pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address critical security and quality findings - Add input validation for bins parameter in histogram functions - Remove 40 binary artifact files from working directory - Replace warning output pollution with silent error handling - Extract magic numbers to named constants for line width, contour levels, and pattern factors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: mark histogram feature as implemented in README Update checkbox to reflect that histogram functionality is complete and functional in the library API. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: prevent segmentation faults in histogram with invalid bins parameter - Move bins validation from setup_histogram_bins to validate_histogram_input - Validate bins > 0 and bins <= MAX_SAFE_BINS before processing - Prevent plot_count increment when validation fails - Add comprehensive test coverage for boundary conditions - Test both figure.hist() and global hist()/histogram() APIs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: update repository references from krystophny/fortplotlib to lazy-fortran/fortplot Updates CMake configuration, documentation URLs, and example references to use correct repository location for CI/CD pipeline functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: remove binary files and build artifacts from workspace - Remove all test output files (*.png, *.pdf, test_*.txt) - Remove build directories and .mod files - Remove macOS artifact file (._.DS_Store) - Ensure clean workspace with no binary files or build artifacts - .gitignore properly configured to prevent future binary commits 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: update CMake example to use correct fortplot target names - Changed fortplotlib to fortplot in FetchContent_Declare - Updated target link from fortplotlib::fortplotlib to fortplot::fortplot - Aligns with actual CMake targets defined in main CMakeLists.txt - Resolves CMake CI build failure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: correct test executable name in CI and improve histogram plot indexing * fix: move histogram test files from app/ to test/ directory for proper organization * fix: resolve segmentation fault in global pyplot-style histogram functions Address critical issue where global hist() and histogram() functions crashed with segmentation fault due to uninitialized global figure backend. Root cause: Global figure variable declared but never auto-initialized when global functions called methods on it. Solution: - Add ensure_global_figure_initialized() helper function - Auto-initialize global figure with default dimensions if backend not allocated - Call helper before hist operations in both hist() and histogram() functions - Maintain matplotlib compatibility by auto-initializing like pyplot.hist() Changes: - src/fortplot.f90: Add auto-initialization to hist() and histogram() - test/test_global_hist_api.f90: Add test for auto-initialization behavior Verification: - Global hist(data) now works without explicit figure() call - Object-oriented fig%hist() continues working unchanged - All existing tests pass - New test verifies matplotlib-compatible auto-initialization 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: correct output paths in boxplot and grid examples - Fix boxplot_demo to use build/example/boxplot_demo/ paths - Fix grid_demo to use build/example/grid_demo/ paths - Resolves runtime failures due to missing plots/ directory - Ensures consistent example output directory structure * fix: resolve histogram allocation error in merged implementation Fixes runtime error when histogram arrays were already allocated: - Add proper deallocation checks before allocating hist_bin_edges - Add proper deallocation checks before allocating hist_counts - Prevents "Attempting to allocate already allocated variable" error This completes the histogram merge integration with main branch. All histogram functionality now works correctly with the new logging system and error bar plotting features. Tested with: - make test (all histogram tests pass) - make example histogram_demo (works correctly) - make example errorbar_demo (main branch features work) - make example bar_chart_demo (main branch features work) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: clean up issue references in README Remove specific issue number references for cleaner documentation. Maintains descriptive text about validation frameworks without coupling to specific issue numbers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: remove obsolete CMake files The project has migrated to FPM build system, making CMake files unnecessary and confusing for new contributors. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: critical array bounds violation in histogram tick calculation - Fix array bounds error at fortplot_ticks.f90:233 when actual_num_ticks = 0 - Add proper bounds checking for edge cases in tick location calculation - Handle zero tick scenarios by falling back to data bounds - Resolves critical runtime bug that blocked all histogram savefig() operations - All histogram functionality now fully operational across PNG, PDF, ASCII backends - Maintains backward compatibility for normal tick calculation scenarios 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: eliminate critical allocation errors in histogram functionality Replace explicit allocate() statements with direct assignment to allocatable arrays. Fortran automatically handles reallocation when using direct assignment, preventing "attempting to allocate already allocated variable" runtime errors. Key changes: - Remove explicit allocate() calls for hist_bin_edges and hist_counts - Use array constructor syntax for cleaner initialization - Ensure zero allocation errors in all histogram operations Fixes all histogram test failures and runtime crashes. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>

qodo-merge-pro bot added the Review effort 3/5 label Jul 20, 2025

krystophny and others added 3 commits July 20, 2025 20:21

krystophny merged commit 0bda9c8 into main Jul 20, 2025
14 checks passed

krystophny deleted the fix-issue-47-disconnected-lines branch July 20, 2025 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #47: Support NaN values for disconnected line segments #48

Fix #47: Support NaN values for disconnected line segments #48

Uh oh!

krystophny commented Jul 20, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

qodo-merge-pro bot commented Jul 20, 2025

Uh oh!

qodo-merge-pro bot commented Jul 20, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jul 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix #47: Support NaN values for disconnected line segments #48

Fix #47: Support NaN values for disconnected line segments #48

Uh oh!

Conversation

krystophny commented Jul 20, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Summary

Implementation Details

Test Plan

Example Usage

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-merge-pro bot commented Jul 20, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

codecov bot commented Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

krystophny commented Jul 20, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Jul 20, 2025 •

edited

Loading

codecov bot commented Jul 20, 2025 •

edited

Loading